feat: add TrustyAI Ragas inline and remote providers#52
feat: add TrustyAI Ragas inline and remote providers#52nathan-weinberg merged 1 commit intoopendatahub-io:mainfrom
Conversation
nathan-weinberg
requested review from
Elbehery,
VaishnaviHire,
cdoern,
derekhiggins,
leseb,
rhdedgar,
rhuss and
skamenan7
as code owners
WalkthroughAdds installation of Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant E as Eval Orchestrator
participant C as Runtime Config (run.yaml)
participant P_inline as trustyai_ragas.inline
participant P_remote as trustyai_ragas.remote
participant KF as Kubeflow Pipelines
participant S3 as S3 Storage
U->>E: Request evaluation
E->>C: Resolve provider & env (uses EMBEDDING_MODEL)
alt inline provider
E->>P_inline: Run eval (embedding_model)
P_inline-->>E: Return results
else remote provider
E->>P_remote: Submit job (embedding_model, kubeflow_config)
P_remote->>KF: Launch pipeline
KF->>S3: Store artifacts (results_s3_prefix)
KF-->>P_remote: Job status & artifact refs
P_remote-->>E: Return results reference
end
E-->>U: Return evaluation outcome
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-15T14:25:54.837ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
distribution/run.yaml (1)
116-127: Gate remote RAGAS on Kubeflow readiness to avoid half-configured providerCurrently gated only by EMBEDDING_MODEL, so the remote provider may be included without Kubeflow settings, leading to runtime failures. Gate inclusion on a Kubeflow var (e.g., KUBEFLOW_PIPELINES_ENDPOINT) as well.
Example change:
- - provider_id: ${env.EMBEDDING_MODEL:+trustyai_ragas_remote} + - provider_id: ${env.KUBEFLOW_PIPELINES_ENDPOINT:+trustyai_ragas_remote}Optionally require both by introducing a dedicated toggle (e.g., USE_TRUSTYAI_RAGAS_REMOTE) or by documenting that both EMBEDDING_MODEL and Kubeflow vars must be set.
Please confirm which Kubeflow fields are mandatory for this provider. If both EMBEDDING_MODEL and KUBEFLOW_PIPELINES_ENDPOINT must be set, we can add a clearer toggle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
distribution/Containerfile(1 hunks)distribution/README.md(1 hunks)distribution/build.yaml(1 hunks)distribution/run.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T14:25:54.837Z
Learnt from: nathan-weinberg
PR: opendatahub-io/llama-stack-distribution#33
File: distribution/Containerfile:17-21
Timestamp: 2025-09-15T14:25:54.837Z
Learning: In the opendatahub-io/llama-stack-distribution repository, the distribution/Containerfile is auto-generated by distribution/build.py based on configuration in build.yaml. When providers are added to build.yaml, the build script automatically regenerates the Containerfile with the required dependencies. Changes to the Containerfile should not be flagged as manual edits if they correspond to legitimate changes in the build configuration.
Applied to files:
distribution/Containerfiledistribution/build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-push (linux/amd64)
- GitHub Check: Summary
🔇 Additional comments (4)
distribution/Containerfile (1)
44-45: Adds required RAGAS provider dependency — looks goodVersion pin matches run.yaml and build.yaml.
Please confirm if 0.1.1 is the intended minimum and compatible with llama-stack==0.2.22.
distribution/README.md (2)
14-14: Docs entry for inline RAGAS added — OKReflects the new inline provider.
16-16: Docs entry for remote RAGAS added — OKReflects the new remote provider.
distribution/run.yaml (1)
111-116: Inline RAGAS provider wiring looks correctGates on EMBEDDING_MODEL and pins module 0.1.1. Note: build.yaml currently uses different provider_type strings; fix there for consistency.
Ensure EMBEDDING_MODEL is always set when enabling this provider to avoid invalid config.
nathan-weinberg
force-pushed
the
ragas
branch
2 times, most recently
from
db1ba45 to
c5e02ed
Compare
nathan-weinberg
added
the
do-not-merge
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
distribution/run.yaml (2)
116-128: Gate remote RAGAS provider on Kubeflow endpoint (safer activation).Currently it’s gated on EMBEDDING_MODEL; if Kubeflow isn’t configured, enabling a remote provider may cause startup or runtime failures. Prefer gating on KUBEFLOW_PIPELINES_ENDPOINT (or a relevant required knob).
Apply this minimal change:
- - provider_id: ${env.EMBEDDING_MODEL:+trustyai_ragas_remote} + - provider_id: ${env.KUBEFLOW_PIPELINES_ENDPOINT:+trustyai_ragas_remote}
199-203: Parameterize embedding config values
Use environment variables for embedding_dimension, model_id, and provider_model_id to avoid mismatches when overriding EMBEDDING_MODEL.- embedding_dimension: 768 - model_id: granite-embedding-125m + embedding_dimension: ${env.EMBEDDING_DIMENSION:=768} + model_id: ${env.EMBEDDING_MODEL:=granite-embedding-125m} provider_id: sentence-transformers - provider_model_id: ibm-granite/granite-embedding-125m-english + provider_model_id: ${env.EMBEDDING_PROVIDER_MODEL_ID:=ibm-granite/granite-embedding-125m-english}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/redhat-distro-container.yml(1 hunks)distribution/Containerfile(1 hunks)distribution/README.md(1 hunks)distribution/build.yaml(1 hunks)distribution/run.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T14:25:54.837Z
Learnt from: nathan-weinberg
PR: opendatahub-io/llama-stack-distribution#33
File: distribution/Containerfile:17-21
Timestamp: 2025-09-15T14:25:54.837Z
Learning: In the opendatahub-io/llama-stack-distribution repository, the distribution/Containerfile is auto-generated by distribution/build.py based on configuration in build.yaml. When providers are added to build.yaml, the build script automatically regenerates the Containerfile with the required dependencies. Changes to the Containerfile should not be flagged as manual edits if they correspond to legitimate changes in the build configuration.
Applied to files:
distribution/Containerfiledistribution/build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-push (linux/amd64)
- GitHub Check: Summary
🔇 Additional comments (4)
distribution/build.yaml (1)
24-27: Normalize theprovider_typenames to match runtime config.
distribution/run.yamldeclares these providers asinline::trustyai_ragas/remote::trustyai_ragas, but here they’re registered asinline::trustyai_ragas_inline/remote::trustyai_ragas_remote. That mismatch breaks the one-to-one mapping the build tooling expects between provider types and the modules we ship, which can lead to stale docs and harder-to-track provider metadata. Please align the strings.- - provider_type: inline::trustyai_ragas_inline + - provider_type: inline::trustyai_ragas module: llama_stack_provider_ragas==0.1.1 - - provider_type: remote::trustyai_ragas_remote + - provider_type: remote::trustyai_ragas module: llama_stack_provider_ragas==0.1.1distribution/README.md (1)
14-17: Docs updated to list RAGAS providers — LGTM.New inline and remote eval providers are reflected correctly. File is auto‑generated.
distribution/run.yaml (1)
111-116: Inline RAGAS provider addition — LGTM; ensure model exists.Config looks correct. Just ensure the EMBEDDING_MODEL value is defined in the models section so provider resolution doesn’t fail at startup.
distribution/Containerfile (1)
44-45: Approve RAGAS provider dependency pin0.1.1 is available on PyPI and consistently used across all distribution files.
distribution/Containerfile
Outdated
| RUN pip install \ | ||
| llama_stack_provider_lmeval==0.2.4 | ||
| RUN pip install \ | ||
| llama_stack_provider_ragas==0.1.1 |
There was a problem hiding this comment.
once trustyai-explainability/llama-stack-provider-ragas#17 is merged this will need to be updated to ~ 0.2.0
There was a problem hiding this comment.
this is now llama-stack-provider-ragas==0.3.0
There was a problem hiding this comment.
@dmaniloff thanks for your effort on trustyai-explainability/llama-stack-provider-ragas#17
Just curious, if this provider is compatible with LLS 0.2.23 ?
cc @m-misiura
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/redhat-distro-container.yml(1 hunks)distribution/Containerfile(1 hunks)distribution/README.md(1 hunks)distribution/build.yaml(1 hunks)distribution/run.yaml(1 hunks)tests/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- distribution/README.md
- .github/workflows/redhat-distro-container.yml
- distribution/Containerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T14:25:54.837Z
Learnt from: nathan-weinberg
PR: opendatahub-io/llama-stack-distribution#33
File: distribution/Containerfile:17-21
Timestamp: 2025-09-15T14:25:54.837Z
Learning: In the opendatahub-io/llama-stack-distribution repository, the distribution/Containerfile is auto-generated by distribution/build.py based on configuration in build.yaml. When providers are added to build.yaml, the build script automatically regenerates the Containerfile with the required dependencies. Changes to the Containerfile should not be flagged as manual edits if they correspond to legitimate changes in the build configuration.
Applied to files:
distribution/build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-push (linux/amd64)
- GitHub Check: Summary
|
This pull request has merge conflicts that must be resolved before it can be merged. @nathan-weinberg please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/redhat-distro-container.yml(1 hunks)distribution/Containerfile(1 hunks)distribution/README.md(1 hunks)distribution/build.yaml(1 hunks)distribution/run.yaml(1 hunks)tests/smoke.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- distribution/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/redhat-distro-container.yml
- distribution/Containerfile
- distribution/build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-push (linux/amd64)
- GitHub Check: Summary
cdoern
left a comment
cdoern
left a comment
There was a problem hiding this comment.
besides the question on the build yaml, this lgtm
| eval: | ||
| - provider_type: remote::trustyai_lmeval | ||
| module: llama_stack_provider_lmeval==0.2.4 | ||
| - provider_type: inline::trustyai_ragas |
There was a problem hiding this comment.
this only adds inline, right? what about remote::trustyai_ragas?
There was a problem hiding this comment.
Yup, asked the Trusty team about that in Slack
There was a problem hiding this comment.
this will install the package with the additional remote deps so it will add both inline and remote providers.
There was a problem hiding this comment.
Are you talking about the module line? Because my concern is around the provider_type line
|
This pull request has merge conflicts that must be resolved before it can be merged. @nathan-weinberg please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
distribution/README.md (1)
16-18: Document all required env vars for remote Ragas.The remote provider still reads
EMBEDDING_MODEL, so folks enabling it must set bothEMBEDDING_MODELandKUBEFLOW_LLAMA_STACK_URL. Please clarify the row to list both requirements to avoid misconfigurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/redhat-distro-container.yml(1 hunks)distribution/Containerfile(1 hunks)distribution/README.md(1 hunks)distribution/build.yaml(1 hunks)distribution/run.yaml(1 hunks)tests/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- distribution/build.yaml
- .github/workflows/redhat-distro-container.yml
- tests/smoke.sh
- distribution/Containerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-push (linux/amd64)
- GitHub Check: Summary
|
cc @ruivieira for context |
| config: | ||
| use_k8s: ${env.TRUSTYAI_LMEVAL_USE_K8S:=true} | ||
| base_url: ${env.VLLM_URL:=} | ||
| - provider_id: ${env.EMBEDDING_MODEL:+trustyai_ragas_inline} |
There was a problem hiding this comment.
| - provider_id: ${env.EMBEDDING_MODEL:+trustyai_ragas_inline} | |
| - provider_id: ${env.EMBEDDING_MODEL:+trustyai_ragas} |
| module: llama_stack_provider_ragas.inline | ||
| config: | ||
| embedding_model: ${env.EMBEDDING_MODEL:=} | ||
| - provider_id: ${env.KUBEFLOW_LLAMA_STACK_URL:+trustyai_ragas_remote} |
There was a problem hiding this comment.
| - provider_id: ${env.KUBEFLOW_LLAMA_STACK_URL:+trustyai_ragas_remote} | |
| - provider_id: ${env.KUBEFLOW_LLAMA_STACK_URL:+trustyai_ragas} |
There was a problem hiding this comment.
What happens if both providers are enabled? Will this still work?
There was a problem hiding this comment.
Good point.
Yes it will work, but users will not have a way to differentiate when doing client.benchmarks.register. I tested this and the provider to be added last via run.yaml will override the previous one.
Should we bring back the suffix to better support the case of both providers enabled?
There was a problem hiding this comment.
Yes, and I'm going to leave it in the YAML for now
Signed-off-by: Nathan Weinberg <nweinber@redhat.com>
nathan-weinberg
removed
the
do-not-merge
|
LGTM actually I already adjusted downstream MR accordingly |
4 participants
Summary by CodeRabbit
New Features
Configuration
Documentation
Chores